Conversation
Greptile SummaryThis PR adds Euclidean one-to-many distance computation with SIMD optimizations (AVX2, AVX512F, AVX512FP16) for float32, float16, and int8 data types. However, multiple critical bugs exist in the new implementation files that will cause incorrect distance calculations:
These bugs were already identified in previous review threads and remain unfixed. Additionally:
Confidence Score: 0/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[BaseDistance::ComputeBatch] --> B{Distance Type?}
B -->|Euclidean| C[EuclideanDistanceBatch::ComputeBatch]
B -->|SquaredEuclidean| D[SquaredEuclideanDistanceBatch::ComputeBatch]
B -->|Other| E[Default _ComputeBatch]
D --> F{Data Type?}
F -->|float| G[SquaredEuclideanDistanceBatchImpl float]
F -->|int8_t| H[SquaredEuclideanDistanceBatchImpl int8_t]
F -->|Float16| I[SquaredEuclideanDistanceBatchImpl Float16]
F -->|Other| J[Fallback Implementation]
G --> K{CPU Features?}
K -->|AVX512F| L[compute_one_to_many_squared_euclidean_avx512f_fp32]
K -->|AVX2| M[compute_one_to_many_squared_euclidean_avx2_fp32]
K -->|None| J
H --> N{CPU Features?}
N -->|AVX2| O[compute_one_to_many_squared_euclidean_avx2_int8]
N -->|None| J
I --> P{CPU Features?}
P -->|AVX512FP16| Q[compute_one_to_many_squared_euclidean_avx512fp16_fp16]
P -->|AVX512F| R[compute_one_to_many_squared_euclidean_avx512f_fp16]
P -->|AVX2| S[compute_one_to_many_squared_euclidean_avx2_fp16]
P -->|None| J
C --> T[Call SquaredEuclideanDistanceBatch]
T --> U[Apply sqrt to results]
style L fill:#f99,stroke:#333,stroke-width:2px
style M fill:#f99,stroke:#333,stroke-width:2px
style O fill:#f99,stroke:#333,stroke-width:2px
style Q fill:#f99,stroke:#333,stroke-width:2px
style R fill:#f99,stroke:#333,stroke-width:2px
style S fill:#f99,stroke:#333,stroke-width:2px
Last reviewed commit: b766fb1 |
src/ailego/math_batch/euclidean_distance_batch_impl_fp32_avx2.cc
Outdated
Show resolved
Hide resolved
|
@greptile |
|
@greptile |
| @@ -189,22 +189,6 @@ class HammingMetric : public IndexMetric { | |||
| return nullptr; | |||
There was a problem hiding this comment.
HammingMetric::batch_distance() removed — now always returns nullptr
The PR removes the entire batch_distance() override from HammingMetric. The base class IndexMetric::batch_distance() returns nullptr by default (see src/include/zvec/core/framework/index_metric.h:85), so callers that rely on this method to get a valid batch-distance function for binary data will now silently receive a null function pointer, potentially causing null-pointer dereferences at runtime.
Additionally, the same PR removes the DT_BINARY32 and DT_BINARY64 cases from SquaredEuclideanMetric::batch_distance() in euclidean_metric.cc (lines 853–862 before this change). Both removals appear unrelated to the stated goal of adding Euclidean one-to-many and should be reverted unless there is an explicit intent to drop batch-distance support for binary metrics.
add euclidean one2many
Greptile Summary
This PR adds SIMD-accelerated Euclidean (L2) one-to-many distance computation to the
zvecbatch distance infrastructure, mirroring the existing inner-product and cosine batch distance paths. It introduces new dispatch and implementation files for FP32/FP16 across AVX2, AVX-512F, and AVX-512FP16 instruction sets, wires them intoBaseDistance::ComputeBatchviaif constexpr, and addsEuclideanMetric::batch_distance().Key issues found:
Wrong distance formula in
euclidean_distance_batch_impl_fp32_avx512.cc(lines 66–69): The tail-element path uses_mm512_mask3_fmadd_ps(q, ptrs[i], accs[i], mask), which accumulates an inner product (q·d) instead of the squared difference(q−d)². Dimensions not a multiple of 16 will return completely incorrect distances on AVX-512F hardware.Wrong distance formula in
euclidean_distance_batch_impl_fp16_avx512.cc(line 77): The intermediate chunk for 16–31 remaining FP16 elements uses_mm512_fmadd_ps(q, data_regs[i], accs[i])(inner product) rather thansub→fmadd(diff, diff, ...). Dimensions in the range(16, 32)modulo 32 are affected.Unrelated regression —
HammingMetric::batch_distance()removed (hamming_metric.cc): The method override is deleted without replacement. The base-class default returnsnullptr, silently breaking any caller that invokes batch distance for binary data through aHammingMetric. The relatedDT_BINARY32/DT_BINARY64cases were also dropped fromSquaredEuclideanMetric::batch_distance()ineuclidean_metric.cc.Tests silenced with
#if 0(hnsw_streamer_test.cc):TestBinaryConverterandTestBasicRefinerare unconditionally disabled, masking the regressions above. These should be fixed rather than suppressed.Confidence Score: 1/5
HammingMetric::batch_distance()and binary-type cases inSquaredEuclideanMetricappears to be an unintended side-effect, and the two disabled tests hide these regressions from CI.Important Files Changed
fmadd_ps(q, data, acc)) instead of squared difference; the rest of the implementation is correct.ptrs[i]dim+[3]on line 74 (compile failure); the main AVX2 loop is correctly implemented using sub + fmadd.SquaredEuclideanDistanceBatch/EuclideanDistanceBatch; theEuclideanDistanceBatchcorrectly appliessqrtafter the squared-distance computation.EuclideanMetric::batch_distance()(correct); also removes binary-type cases fromSquaredEuclideanMetric::batch_distance(), which is an unintended regression.HammingMetric::batch_distance()entirely; the base class returnsnullptr, breaking batch binary distance computation.TestBinaryConverter,TestBasicRefiner) are disabled with#if 0, hiding potential regressions from the binary-metric removals.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["BaseDistance::ComputeBatch()"] --> B{DistanceType?} B -- "EuclideanDistanceMatrix" --> C["EuclideanDistanceBatch::ComputeBatch()"] B -- "SquaredEuclideanDistanceMatrix" --> D["SquaredEuclideanDistanceBatch::ComputeBatch()"] B -- "other" --> E["_ComputeBatch() fallback"] C --> F["SquaredEuclideanDistanceBatch::ComputeBatch()"] F --> G["sqrt(results[i]) for each i"] D --> H["SquaredEuclideanDistanceBatchImpl::compute_one_to_many()"] H --> I{CPU feature?} I -- "AVX-512FP16 (FP16 only)" --> J["avx512fp16_fp16 impl\n✅ correct"] I -- "AVX-512F (FP16)" --> K["avx512f_fp16 impl\n⚠️ line 77: inner product bug"] I -- "AVX-512F (FP32)" --> L["avx512f_fp32 impl\n⚠️ lines 66-69: inner product bug"] I -- "AVX2 (FP16)" --> M["avx2_fp16 impl\n✅ correct"] I -- "AVX2 (FP32)" --> N["avx2_fp32 impl\n⚠️ line 74: syntax error"] I -- "none" --> O["fallback scalar impl\n✅ correct"]Comments Outside Diff (1)
tests/core/algorithm/hnsw/hnsw_streamer_test.cc, line 3500 (link)#if 0TestBinaryConverterandTestBasicRefinerare both silenced with#if 0in this PR. Permanently disabling tests with preprocessor guards makes failures invisible and can hide regressions. If these tests are failing due to the binary-type removals inhamming_metric.cc/euclidean_metric.cc, the root cause should be fixed rather than the tests suppressed. If the tests are being temporarily skipped for another reason, usingDISABLED_as a Google Test prefix is the conventional way to do this without removing coverage entirely.Last reviewed commit: "fix: fix format"